Added the optimized version on top of this PR:
w0xlt@8d16914

For more context:
#1698 (comment)

Small supplementary update: I've created a corresponding Python implementation of the provided API functions based on secp256k1lab (https://github.com/theStack/secp256k1lab/blob/add_bip352_module_review_helper/src/secp256k1lab/bip352.py) (also linked in the PR description). The hope is that this makes reviewing this PR a bit easier by having a less noisy, "executable pseudo-code"-like description on what happens under the hood. The code passes the BIP352 test vectors and hence should be correct.

Added the optimized version on top of this PR: w0xlt@8d16914

For more context: #1698 (comment)

Thanks for rebasing on top of this PR, much appreciated! I will take a closer look within the next days.

Not providing prevouts_summary (de)serialization functionality yet in the API poses the risk that users try to do it anyway by treating the opaque object as "serialized". How to cope with that? Is adding a "don't do this" comment in API header sufficient?

Is there a reason for serializing prevouts_summary without light client functionality? If not, I think the don't do this comment is sufficient. Right now, in contrast to the docs of all other opaque objects, this is missing, however:

The exact representation of data inside the opaque data structures is implementation defined and not guaranteed to be portable between different platforms or versions.

@w0xlt, @jonasnick: Thanks for the reviews! I've addressed the suggested changes:

• in _recpient_scan_outputs: changed the type of k to uint32_t (comment above)
• in _recipient_create_label: added a scan key validity check (+added a test for that) (#1698 - comment)
• unified all mentions of "Silent Payments" to title case in the header API and example (#1698 - comment)
• fixed typo s/elemement/element/ (#1698 - review)
• in _recipient_scan_outputs: fixed comment in second label candidate (review above)
• extended the API header comment for the _prevouts_summary opaque data structure, to point out that the data structure is implementation defined (like docs of all other opaque structs) (comment above)

Nit: Not related to optimization, but the diff below removes some redundant public-key serialization code:

Given that this compressed-pubkey-serialization pattern shows up repeatedly also in other modules (ellswift, musig), I think it would make the most sense to add a general helper (e.g. in eckey{,_impl}.h), which could be done in an independent PR. I've opened issue #1773 to see if there is conceptual support for doing this.

Not providing prevouts_summary (de)serialization functionality yet in the API poses the risk that users try to do it anyway by treating the opaque object as "serialized". How to cope with that? Is adding a "don't do this" comment in API header sufficient?

Is there a reason for serializing prevouts_summary without light client functionality? If not, I think the don't do this comment is sufficient.

Good point, I can't think of a good reason for full nodes wanting to serialize prevouts_summary.

To address the open questions, I’ve reviewed the proposed changes by @w0xlt on 8d16914.

I'm going to focus more on the key aspects I extracted from the review and the merits of each change, rather on the big O improvement claims, because I didn't get that far.

These are multiple different changes rather than a single one, so to make the review easier I suggest to brake it in multiple commits. I would state on each of them the purpose and the real case scenario where the change would be relevant.

Also, I would use clearer names for the variables or at least document their purpose.

The changes I've identified so far are the following:

• Improve label lookup using hash table: I think this is implementation dependent and should be improved by the user rather than by the library itself.
  Examples, as part of the documentation, are a usage demonstration, although can point the user the best practices, I prefer clarity rather than performance on them. For example, on the rust-secp256k1 bindings, I used a HashMap for the example because it is a familiar standard structure for Rust users. If they would like to gain more performance there, they have other tools available to replace that structure by themselves.
• On secp256k1_silentpayments_recipient_sort_cmp, I understood the change: (r1->index < r2->index) ? -1 : (r1->index > r2->index) ? 1 : 0 is to make the unstable secp256k1_hsort implementation stable. Considering it only affects secp256k1_silentpayments_sender_create_outputs, which didn't receive more changes than a variable removal, what are the performance improvements there?
• SECP256K1_SP_SCAN_BATCH: I just learned about it (ge_set_gej_all) during this review, but seems that affine to jacobian conversion is needed for the comparison against labels, and is faster to do it in batches. I think the impact of this improvement will only affect the small subset of transaction with large amount of outputs. A good benchmark for this would be coinjoin transactions, although is not supported by BIP 352, a test case is doable.
• Double head: I'm not sure of the target of this change, I guess is to skip already found or scanned inputs, but couldn't figure out what is tracking each head.
• Binary tree search for x-only lookups: I think it explain itself, faster lookups on the not labeled case. This may have its merits, but I need to remove the other changes to have a clear answer.

In general I agree with @jonasnick that we should define a clear target to benchmark and improve. As I've said before, the base case should be a wallet with a single label for change.
For other improvements, I would try to match up plausible real world scenarios before making complex changes in the code base of the PR.
Finally, by looking at bench_silentpayments_full_tx_scan, the use_labels case is very simple. If we want to test performance improvements on the label lookup, I would start there.

In conclusion, from the proposed commit and the discussion around it, the only changes I've found clear enough to consider are:

• Tracking of found outputs: simple enough, small performance improvement for the usual case, better for larger transactions.

Thanks @nymius for reviewing the changes, addressing the main points, and proposing a simplification.

I’m currently splitting the optimization commit into smaller pieces to make it easier to review.
I’ll also take a closer look at your commit and run it against the benchmark files.

The only part of the discussion that still feels a bit ambiguous is the “base” or “usual” case.
From my understanding of #1698 (review), the concern is not about typical usage, but rather about an attacker crafting malicious transactions with many outputs, causing the scanning process to take hours.

So the goal of this optimization would be to mitigate that scenario, not the collaborative one.

I ran the examples/silentpayments_mixed_1.c file with simplified the version suggested by @nymius jonasnick@311b4eb . It shows slightly worse performance (see below), but the simpler approach may still be worth it

Without the secp256k1_silentpayments_recipient_sort_cmp stabilization, I got 82s for the complex version vs. 114s for the simpler one. Whether the 30s difference justifies the additional complexity is up for discussion — I don’t have a strong opinion.

Answering the questions: secp256k1_silentpayments_recipient_sort_cmp stabilization + heads speed up the non-adversarial case. The stable sort ensures that the transaction outputs are ordered sequentially by the index $k$.

The optimized receiver implementation relies on a heuristic (the head pointers) that assumes the next output it is looking for ($k+1$) is located immediately after the previous one ($k$).

• With Stable Sort: The scanner complexity is roughly O(N) (Linear).
• Without Stable Sort: The scanner complexity degrades to O(N²) (Quadratic).

@w0xlt, @nymius: Thanks for investigating this deeper. I've now also had a chance to look at the suggested optimizations and came to similar conclusions as stated in #1765 (comment). I particularly agree with the stated points that the changes should not increase complexity significantly and that the most important optimization candidate to consider for mitigating the worst-case scanning attack is "skip outputs that we have already found" (as previously stated by @jonasnick, see #1698 (comment) and jonasnick@311b4eb). I don't think stabilizing the sorting helps at all, since this is something that happens at the sender side, and we can't rely on the attacker using a specific implementation (even if they did, it's trivial for them to shuffle the outputs after creation).

For the proposed target to benchmark, I'm proposing the following modified example that exhibits the worst-case scanning time based on a labels cache with one entry (for change outputs), by creating a tx with 23255 outputs [1] all targeted for Bob: 1df4287

Shower-thought from this morning: what if we treat the tx_outputs input as actual list that we modify, and remove an entry if it is found? This would have a similar effect as the "track found outputs" idea, but without the need of dynamic memory allocation. It's a ~10-lines diff and seems to work fine: theStack@9aba470
It reduces the run-time of the proposed example above from ~10 minutes to roughly ~2 seconds on my machine.

Any thoughts on this? Maybe I'm still missing something.

[1] that's an upper bound of maximum outputs per block: floor(1000000/43) = 23255

Shower-thought from this morning: what if we treat the tx_outputs input as actual list that we modify, and remove an entry if it is found? This would have a similar effect as the "track found outputs" idea, but without the need of dynamic memory allocation. It's a ~10-lines diff and seems to work fine: theStack@9aba470 It reduces the run-time of the proposed example above from ~10 minutes to roughly ~2 seconds on my machine.

Any thoughts on this? Maybe I'm still missing something.

1df4287 is a good target.
These are the runtime times I've obtained testing against this target:

I had to increase stack size to be able to fit all N_OUTPUT size allocations in the example.

Initially I preferred the is_found allocation rather than the element shifts. But your solution seems to be more performant.

@theStack Yes — if we want to keep only the adversarial-scenario optimizations, we can drop sort stabilization and the extra heads.

I like your idea of avoiding dynamic memory allocation. That’s a very interesting direction. On my machine, the scan completes in about 0.4s, which feels like a good balance between simplicity and the optimization needed for the labeled case.

Below are the changes I had to make for your example to run on my machine and to record the scan time.

diff --git a/examples/silentpayments.c b/examples/silentpayments.c
index 5e71e73..d43332f 100644
--- a/examples/silentpayments.c
+++ b/examples/silentpayments.c
@@ -10,6 +10,7 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <time.h>
 
 #include <secp256k1_extrakeys.h>
 #include <secp256k1_silentpayments.h>
@@ -112,15 +113,21 @@ const unsigned char* label_lookup(
     return NULL;
 }
 
+static secp256k1_xonly_pubkey tx_inputs[N_INPUTS];
+static const secp256k1_xonly_pubkey *tx_input_ptrs[N_INPUTS];
+static secp256k1_xonly_pubkey tx_outputs[N_OUTPUTS];
+static secp256k1_xonly_pubkey *tx_output_ptrs[N_OUTPUTS];
+static secp256k1_silentpayments_found_output found_outputs[N_OUTPUTS];
+static secp256k1_silentpayments_found_output *found_output_ptrs[N_OUTPUTS];
+static secp256k1_silentpayments_recipient recipients[N_OUTPUTS];
+static const secp256k1_silentpayments_recipient *recipient_ptrs[N_OUTPUTS];
+/* 2D array for holding multiple public key pairs. The second index, i.e., [2],
+ * is to represent the spend and scan public keys. */
+static unsigned char (*sp_addresses[N_OUTPUTS])[2][33];
+
 int main(void) {
     unsigned char randomize[32];
     unsigned char serialized_xonly[32];
-    secp256k1_xonly_pubkey tx_inputs[N_INPUTS];
-    const secp256k1_xonly_pubkey *tx_input_ptrs[N_INPUTS];
-    secp256k1_xonly_pubkey tx_outputs[N_OUTPUTS];
-    secp256k1_xonly_pubkey *tx_output_ptrs[N_OUTPUTS];
-    secp256k1_silentpayments_found_output found_outputs[N_OUTPUTS];
-    secp256k1_silentpayments_found_output *found_output_ptrs[N_OUTPUTS];
     secp256k1_silentpayments_prevouts_summary prevouts_summary;
     secp256k1_pubkey unlabeled_spend_pubkey;
     struct labels_cache bob_labels_cache;
@@ -209,11 +216,6 @@ int main(void) {
     {
         secp256k1_keypair sender_keypairs[N_INPUTS];
         const secp256k1_keypair *sender_keypair_ptrs[N_INPUTS];
-        secp256k1_silentpayments_recipient recipients[N_OUTPUTS];
-        const secp256k1_silentpayments_recipient *recipient_ptrs[N_OUTPUTS];
-        /* 2D array for holding multiple public key pairs. The second index, i.e., [2],
-         * is to represent the spend and scan public keys. */
-        unsigned char (*sp_addresses[N_OUTPUTS])[2][33];
         unsigned char seckey[32];
 
         printf("Sending...\n");
@@ -340,6 +342,9 @@ int main(void) {
              *        `secp256k1_silentpayments_recipient_prevouts_summary_create`
              *     2. Call `secp256k1_silentpayments_recipient_scan_outputs`
              */
+            clock_t start, end;
+            double cpu_time_used;
+
             ret = secp256k1_silentpayments_recipient_prevouts_summary_create(ctx,
                 &prevouts_summary,
                 smallest_outpoint,
@@ -356,14 +361,20 @@ int main(void) {
 
             /* Scan the transaction */
             n_found_outputs = 0;
+            
+            start = clock();
             ret = secp256k1_silentpayments_recipient_scan_outputs(ctx,
                 found_output_ptrs, &n_found_outputs,
-                (const secp256k1_xonly_pubkey * const *)tx_output_ptrs, N_OUTPUTS,
+                (const secp256k1_xonly_pubkey **)tx_output_ptrs, N_OUTPUTS,
                 bob_scan_key,
                 &prevouts_summary,
                 &unlabeled_spend_pubkey,
                 label_lookup, &bob_labels_cache /* NULL, NULL for no labels */
             );
+            end = clock();
+            cpu_time_used = ((double) (end - start)) / CLOCKS_PER_SEC;
+            printf("Bob's scan took %f seconds\n", cpu_time_used);
+            
             if (!ret) {
                 printf("This transaction is not valid for Silent Payments, skipping.\n");
                 return EXIT_SUCCESS;
@@ -435,7 +446,7 @@ int main(void) {
             n_found_outputs = 0;
             ret = secp256k1_silentpayments_recipient_scan_outputs(ctx,
                 found_output_ptrs, &n_found_outputs,
-                (const secp256k1_xonly_pubkey * const *)tx_output_ptrs, 1, /* dummy scan with one output (we only care about Bob) */
+                (const secp256k1_xonly_pubkey **)tx_output_ptrs, 1, /* dummy scan with one output (we only care about Bob) */
                 carol_scan_key,
                 &prevouts_summary,
                 &unlabeled_spend_pubkey,

@nymius, @w0xlt: Thanks once again for the quick feedback and for benchmarking! Shortly after my previous comment, I've been notified about yet another approach to tackle the worst-case scanning time attack (kudos to @furszy for bringing up the idea!), that I think is even more elegant: we can use the pointers in the tx_outputs list directly to track outputs by setting them to NULL if one has been found, and accordingly only treat them if they are non-NULL. With this, it's an only four lines of code change: theStack@2087f92. It kind of combines the previous two approaches of jonasnick@311b4eb and theStack@9aba470 (-> mark spent outputs, but not in a newly allocated array, but by modifying the tx_outputs input list, in order to avoid dynamic memory allocation), with very similar run-time results.

The only tiny drawback about these non-malloc approaches might be that something that is conceptually an "in" parameter is modified, which might be a bit unsound in a strict API design sense. On the other hand, it shouldn't matter for the user (I doubt that these lists passed in would ever be reused for anything else after by the callers), and we already do the same in the sending API for the recipients, so it's probably fine.

@theStack Yes — if we want to keep only the adversarial-scenario optimizations, we can drop sort stabilization and the extra heads.

The way I see it currently, code paths for non-adversarial scenarios with increasing k values would be hit so rarely in practice, that I'm sceptical that it's worth it put much effort into those optimizations. When scanning, the vast majority of transactions won't have any matches in the first place. Out of those few that do have a match, the vast majority will very likely again not contain any repeated recipient (IMHO it doesn't make that much sense to do that, unless the recipient explicitly asks "I want to receive my payment split up in multiple UTXOs, but still in a single tx"?), so in the bigger picture those optimizations wouldn't matter all that much, and I'd assume that the dominant factor should be by far all the (unavoidable) ECDH computations per transaction. But that's still more of a guess and it's still good to already have optimization ideas at hand if we need them in the future.

@theStack Thanks for continuing to refine the optimization. The deletion approach performs slightly better (0.40 s vs. 0.45 s), likely because deleting items shrinks the array and cuts the number of loop iterations by about 50% compared to nullifying them.

To summarize, the following table shows the proposed mitigations for the worst-case scanning attack so far, with benchmark results from my machine. The previous baseline commit with the worst-case example has been updated to include @w0xlt's changes, in order to work without stack size limit changes.
(EDIT: These benchmark results are based on a baseline that doesn't represent the worst-case and are thus not representative, as noticed by w0xlt below.)

The run-times of the fixes vary slightly (the removal approach "fix2" being the fastest, confirming #1765 (comment) above), but are all in the same ballpark. I don't think exact performance results matter much here, as the goal of the mitigation should be to IMHO roughly cut the run-time down from "minutes" to "seconds" (and remember, this is already for the absolute worst-case, one giant non-standard transaction filling out a whole block, and it can only slow down one specific SP recipient). Thus, I decided to pick the the simplest approach that avoids dynamic memory allocation, i.e. fix number 3 using NULL as marker in tx_outputs.

With that tackled, I believe that all of the open questions and TODOs are addressed now (updated the PR description accordingly). The latest force-push also includes a rebase on master (to include the CI fix #1771).

I assume you want to rebase on master now that #1774 has been merged.

Yes, done. Using the new secp256k1_eckey_pubkey_serialize33 function, the sending and receiving commit diffs got a bit smaller (as outlined in #1765 (review)), -13 LOC in total.

Thanks for the summary, I've checked the results. I agree with fix3_* branch as the final solution, is elegant and trade-offs are clear.

Hi @theStack . That's not actually the worst case. The worst case is when the attacker shuffles the outputs randomly (or in any adversarial order). I modified your benchmark to randomly shuffle outputs, and scanning takes several minutes to hours with the current optimization proposal.

w0xlt@435cb96

The issue is that the scanning function receives a label lookup callback, which makes it harder to build a sorted index upfront. Instead, for each output tweak index k, it linearly scans all transaction outputs — resulting in O(n²) complexity. An attacker creating ~23,000 unordered outputs can make scanning take hours.

If we slightly change the API to replace the label lookup callback with a label entry set, we can sort once and search fast:

1. Build a sorted index of outputs by serialized x-only pubkey — O(n log n) upfront
2. Binary search for each candidate — O(log n) per lookup


That way, scanning 23,255 adversarially-ordered outputs drops from hours to ~0.3s. The optimization is purely receiver-side, requires no protocol changes, and works regardless of output ordering. This reduces architectural flexibility, though.

Before: for each k, scan all n outputs → O(k × n) ≈ O(n²)
After: sort outputs, binary search → O(n log n + k log n)



The following commit implements this proposal:


w0xlt@2305d73

You can verify by running ./build/bin/silentpayments_example (ordered) and ./build/bin/silentpayments_shuffled_example (shuffled).

Hi @theStack . That's not actually the worst case. The worst case is when the attacker shuffles the outputs randomly (or in any adversarial order).

@w0xlt: Good catch, I indeed missed involving the output order for the worst-case scenario, so theStack@c16252d is not a useful target (in hindsight, the benchmark results look too good to be true, and my previous words of "we can't rely on the attacker using a specific implementation" strike back very hard). Thanks for the updated benchmark and the new worst-case fix proposal!

I modified your benchmark to randomly shuffle outputs, and scanning takes several minutes to hours with the current optimization proposal.

Are you sure it could take up "to hours"? That would be significantly worse than the previous baseline commit (~10 minutes on my machine), which seems implausible to me. In the unoptimized scanning implementation, the run-time should be very much independent on the order of outputs, as the total number of inner loop iterations is constant ($1+2+3...+N = (N*(N+1))/2 = 270409140$ for our worst-case scenario of $N = 23255$), and it'd be very surprising if any of the three proposed optimizations perform worse than the baseline for shuffled outputs.

I only did two runs so far with your shuffle-patch added, and got runtimes of ~10 minutes (unoptimized) and ~5 minutes (NULL-ify patch, "fix3"). Will do some more thorough benchmarks tomorrow, including your proposed solution.

@theStack Thanks for looking into this.
Sorry if I wasn’t clear earlier. The shuffled benchmark does perform better in the optimized version than in the baseline, but it loses the sub-second scan times.
The new proposed approach (assuming it works as expected) should keep those sub-second times even on the shuffled benchmark.

@w0xlt: I've looked at your proposed scanning implementation w0xlt@2305d73 and can confirm that it fixes the worst-case scanning attack, as the run-time of the modified example with shuffled outputs is reduced to <1s. What makes this different approach potentially interesting is that it could also to speed up the common case (i.e. no matches), as the number of point additions needed for labels scanning seems to be significantly smaller for the small-scale user (where N_LABELS < 2 * N_OUTPUTS for the average transaction; if N_LABELS=1 and we assume the average transaction to scan for has N_OUTPUTS=2, your approach is 4x faster w.r.t. needed point additions). To verify that I understood it correctly and make the idea digestible to other interested reviewers, I've tried to summarize the current PR's scanning approach (as proposed in BIP-352) vs. your approach on the following gist, written in a somewhat hand-wavy way, I hope the pseudo-code is understandable: https://gist.github.com/theStack/25c77747838610931e8bbeb9d76faf78. Please let me know if I got that right, I think the approach leads to the same result, but I'm still trying to wrap my head around it.
// EDIT: I've been told by @setavenger that this approach is also used by Frigate Electrum Server and the BlindBit wallet.

On the other hand, I'm still uncertain on whether adding the additional code complexity and the involvement of dynamic memory allocation at all in a secp256k1 module is worth all this, considering that we are talking about an attack that costs an adversary money (at least the fees for the whole block space) and can only slow down one specific SP recipient. It seems a very unusual kind of attack. Changing the scanning API at this point seems also questionable, as I think it went already through several iterations and refinements over the years, and I think the "pass labels directly" approach is slower for users with a higher number of labels involved. Also, the mental load for reviewers is significantly higher if the approach doesn't follow the algorithm as specified in BIP-352, as they have to convince themselves that the result is equivalent.

I'm hence debating on whether one solution could be to keep the PR simply as-is and accept the fact that an adversary can slow down a single entity for a few minutes (for non-standard transactions), to a few seconds (for standard transactions, i.e. tx size <= 100kvB). I don't have a good final answer to this question myself, but want to discuss is at least and come to a decision that both maintainers and users are on board with before taking any further action.

My (in retrospect somewhat naive) hope was that there could be a simple few-lines fix for reducing the worst-case scanning attack down to seconds without the need of dynamic memory allocation, but such an easy fix doesn't seem to be in sight.

Hi @theStack thanks a lot for taking the time to review the proposal and write up the comparison.

My view is that we should aim for the most efficient and resilient solution, even if the attack scenario is somewhat unusual. The spec can be updated as needed to reflect improvements. Independently of adversarial cases, reducing scanning time from several minutes to under a second is a major win for user experience.

Regarding complexity: although this approach adds some moving parts, the underlying idea (e.g., using binary search) is conceptually straightforward, so I don’t think it should pose a significant burden by itself.

As for the dynamic memory concern, the allocation doesn’t necessarily have to happen inside the secp256k1 module. We could let the caller provide the required workspace. For example, could we leverage secp256k1_scratch_space here?

@w0xlt: After tinkering a bit with your proposed implementation w0xlt@2305d73, I found that it could be significantly simplified, and it turned out we don't even need dynamic memory allocation:

• instead of building an explicit index, we can simply sort the user-supplied tx_outputs array of x-only pubkeys in place and perform binary search directly on that (using the existing secp256k1_pubkey_xonly_pubkey_cmp)
• the preparation of the label set group elements array is not needed as well; _pubkey_load is a very cheap operation, so it doesn't hurt to call it repeatedly in each k loop iteration (remember that the common case is not having a match, i.e. k doesn't exceed 0 for most scanned transactions)

Doing this in theStack@1a40e14 still keeps the worst-case scanning benchmark time down at ~450ms on my machine.
Admittedly, there is a tiny bit of run-time overhead introduced with the simplification (repeated x-only pubkey serialization for the common case), but I don't think that matters much, considering that the involved elliptic curve operations are likely orders of magnitude more expensive than that. With these new insights, I can retract my doubts about code complexity and dynamic memory allocation of your proposed approach, and it's good to have two potential scanning implementations. 👍

It should be pointed out that the "label set" approach doesn't come for free though: one major drawback is that for users with a larger label set, the typical scanning scenario (assuming most txs don't have more than just a few outputs) can be slower at some point, as the number of point additions is higher than with the "BIP" approach. So if we follow your approach, it's better for small-case users and to eliminate the worst-case scanning attack, but users with a lot of labels are likely not happy with it and would very much prefer the "BIP" scanning approach. Not really sure how to deal with that and what to prioritize. (Provide two scanning implementations and let the user pick? 😬 )

I'll anyways try to come up with another "modified example" benchmark to better verify the claims about typical scanning scenarios, depending on the number of labels involved and transaction output count. We have spent a good time on the worst-scanning scenario now, but it would be wrong to solely optimize for that and get the typical scanning scenario out of sight.

Independently of adversarial cases, reducing scanning time from several minutes to under a second is a major win for user experience.

I don't agree with that statement, as I can't think of a legit non-adversarial case that would lead to scanning times of several minutes, in particular as this would at the minimum involve creating a large non-standard transaction. When considering user experience, by far the most relevant case I'd consider is the "no match" scenario (one k loop iteration), as it is the most frequent one, followed by the "single match" (two k loop iterations) scenario. Everything above that should be already quite rare in practice.

As for the dynamic memory concern, the allocation doesn’t necessarily have to happen inside the secp256k1 module. We could let the caller provide the required workspace. For example, could we leverage secp256k1_scratch_space here?

Good question. Right now, the scratch space API is not even exposed to the user. Btw I don't think dynamic memory allocation is a strict no-go in general, it just seems that there are still some open questions around that area, and waiting until they are all tackled would probably delay the PR for an unforeseeable time. It seems that as of now we can easily avoid dynamic memory allocation anyways though, in both of the proposed implementations.

Not really sure how to deal with that and what to prioritize. (Provide two scanning implementations and let the user pick? 😬 )

Some (very) rough ideas on the trade-off…

If we can define a general rule that determines when it’s better to use the label-set scan versus the BIP-based scan, we could do something like:

/* Choose algorithm based on this transaction's characteristics */
if (n_labels > 2 * n_tx_outputs) {
    /* BIP approach: iterate over outputs, add, and look up in label callback */
    use_bip_scan();
} else {
    /* Label-set approach: iterate over labels, add, and search in output index */
    use_labelset_scan();
}

The heuristic above is just a rough estimate. The BIP version performs roughly ~2N point additions, where N is the number of outputs, while the label-set version performs about ~L additions, where L is the number of configured labels.

But to build a reliable decision rule, we’d need more benchmark data across a range of (N, L) combinations.

Here is a first attempt at creating a scanning benchmark for the common case scenario (i.e. no matches and hence only one k iteration), comparing the "BIP" and "Label-set" approaches over a combination of L (number of labels to scan for) and N (number of tx outputs): theStack@fef63fd. Running this modified example outputs the following on my arm64 machine:

$ ./build/bin/silentpayments_example
Silent Payments (BIP-352) scanning benchmarks
[common case scenario, i.e. only one k iteration without match]

Legend: L... number of labels, N... number of transaction outputs

===== BIP approach (calculate label candidates for each output, look them up in labels cache) =====
L= 1: [N=2:  69 us] [N=5:  82 us] [N=10:  89 us] [N=20: 121 us] [N=50: 197 us] [N=100: 343 us] [N=200: 632 us]
L= 2: [N=2:  61 us] [N=5:  80 us] [N=10:  78 us] [N=20: 101 us] [N=50: 177 us] [N=100: 314 us] [N=200: 566 us]
L= 3: [N=2:  54 us] [N=5:  58 us] [N=10:  76 us] [N=20:  95 us] [N=50: 166 us] [N=100: 288 us] [N=200: 535 us]
L= 5: [N=2:  51 us] [N=5:  55 us] [N=10:  67 us] [N=20: 160 us] [N=50: 159 us] [N=100: 271 us] [N=200: 496 us]
L=10: [N=2:  49 us] [N=5:  55 us] [N=10:  66 us] [N=20:  89 us] [N=50: 158 us] [N=100: 276 us] [N=200: 494 us]
L=20: [N=2:  54 us] [N=5:  55 us] [N=10:  68 us] [N=20:  90 us] [N=50: 159 us] [N=100: 271 us] [N=200: 505 us]
L=50: [N=2:  49 us] [N=5:  55 us] [N=10:  71 us] [N=20:  88 us] [N=50: 160 us] [N=100: 274 us] [N=200: 504 us]

===== Label-set approach (calculate output candidate for each label, look it up in outputs) =====
L= 1: [N=2:  51 us] [N=5:  47 us] [N=10:  48 us] [N=20:  51 us] [N=50:  59 us] [N=100:  77 us] [N=200: 116 us]
L= 2: [N=2:  49 us] [N=5:  57 us] [N=10:  50 us] [N=20:  53 us] [N=50:  65 us] [N=100:  81 us] [N=200: 120 us]
L= 3: [N=2:  49 us] [N=5:  51 us] [N=10:  50 us] [N=20:  56 us] [N=50:  63 us] [N=100:  82 us] [N=200: 118 us]
L= 5: [N=2:  58 us] [N=5:  52 us] [N=10:  55 us] [N=20:  56 us] [N=50:  68 us] [N=100:  81 us] [N=200: 119 us]
L=10: [N=2:  63 us] [N=5:  59 us] [N=10:  61 us] [N=20:  62 us] [N=50:  71 us] [N=100:  87 us] [N=200: 126 us]
L=20: [N=2:  70 us] [N=5:  71 us] [N=10:  74 us] [N=20:  90 us] [N=50:  83 us] [N=100: 101 us] [N=200: 139 us]
L=50: [N=2: 104 us] [N=5: 107 us] [N=10: 111 us] [N=20: 113 us] [N=50: 122 us] [N=100: 141 us] [N=200: 192 us]

These results should be taken with a large grain of salt, as the numbers fluctuate a lot (visible for the BIP approach, where all lines should have about the same results, as the performance is in theory independent of L; also, for the first two L the results are surprisingly worse, maybe some unintended caching effects for later runs?) but one can at least see the general previously suspected trend that the BIP approach gets worse with increasing N, while the label-set approach gets worse with increasing L. As a very rough first statement, I'd say assuming that users don't use more than a handful (let's say up to 10) labels and transactions with more outputs get more common in the future, using the label-set approach seems superior, and the only reason to keep the BIP approach is to target users that want to scan for a large number of (let's say dozens) of labels.

Comments or ideas on how to best reason about this would be much appreciated (also a rough review of the modified benchmark example, it could also be that there are bugs). Maybe someone has an idea how to connect this with some actual historic on-chain stats? The list of L and N values to benchmark for can be adapted by changing the two arrays n_labels_bench and n_outputs_bench at the top of the example file.

If we can define a general rule that determines when it’s better to use the label-set scan versus the BIP-based scan, we could do something like:

Makes sense yeah, thought about something similar. What makes this a bit inconvenient (aside from additional review and maintenance burden, obviously) is that users would have to pass both the labels set and implement and pass in a call-back functions, which seems to result in a bloated API. Maybe it's better to focus on one approach and ship another scan function for "giant label set" power users later? I could be wrong, but I don't think using more than three or four labels would be a very common use-case.

@theStack Thanks very much for this benchmark. It is very insightful.

From my understanding, your benchmark confirms that the label-set approach performs better overall and scales more smoothly with the transaction size N (number of outputs). For small N they are comparable, but as N grows the label-set approach becomes significantly faster than the BIP approach.

I also evaluated Montgomery batch inversion via secp256k1_ge_set_all_gej_var in the label-set scanning path on top of your commit. This seems to mainly help when there are many labels (large L). For L = 50 the label-set approach becomes noticeably faster still, so it dominates the BIP-style scanning for almost all (L, N) combinations in this benchmark. All runs are using the same “common case” scenario as in your example (one k iteration, no match).

w0xlt@1e8d196

Below is the benchmark commit as measured on my machine (no batch inversion):

===== BIP approach (calculate label candidates for each output, look them up in labels cache) =====
L= 1: [N=2: 351 us] [N=5: 200 us] [N=10: 231 us] [N=20: 261 us] [N=50: 289 us] [N=100: 451 us] [N=200: 745 us]
L= 2: [N=2:  81 us] [N=5: 109 us] [N=10:  82 us] [N=20: 153 us] [N=50: 218 us] [N=100: 379 us] [N=200: 631 us]
L= 3: [N=2:  55 us] [N=5:  59 us] [N=10:  75 us] [N=20:  99 us] [N=50: 185 us] [N=100: 299 us] [N=200: 586 us]
L= 5: [N=2:  52 us] [N=5:  58 us] [N=10:  70 us] [N=20:  90 us] [N=50: 171 us] [N=100: 302 us] [N=200: 514 us]
L=10: [N=2:  48 us] [N=5:  53 us] [N=10:  62 us] [N=20:  86 us] [N=50: 153 us] [N=100: 269 us] [N=200: 489 us]
L=20: [N=2:  44 us] [N=5:  52 us] [N=10:  64 us] [N=20:  86 us] [N=50: 147 us] [N=100: 267 us] [N=200: 481 us]
L=50: [N=2:  44 us] [N=5:  47 us] [N=10:  61 us] [N=20:  91 us] [N=50: 153 us] [N=100: 257 us] [N=200: 469 us]

===== Label-set approach (calculate output candidate for each label, look it up in outputs) =====
L= 1: [N=2:  39 us] [N=5:  38 us] [N=10:  37 us] [N=20:  38 us] [N=50:  41 us] [N=100:  48 us] [N=200:  64 us]
L= 2: [N=2:  37 us] [N=5:  36 us] [N=10:  36 us] [N=20:  37 us] [N=50:  41 us] [N=100:  46 us] [N=200:  65 us]
L= 3: [N=2:  40 us] [N=5:  36 us] [N=10:  37 us] [N=20:  39 us] [N=50:  43 us] [N=100:  49 us] [N=200:  75 us]
L= 5: [N=2:  39 us] [N=5:  39 us] [N=10:  41 us] [N=20:  42 us] [N=50:  47 us] [N=100:  51 us] [N=200:  84 us]
L=10: [N=2:  47 us] [N=5:  47 us] [N=10:  45 us] [N=20:  46 us] [N=50:  49 us] [N=100:  55 us] [N=200:  71 us]
L=20: [N=2:  58 us] [N=5:  56 us] [N=10:  58 us] [N=20:  55 us] [N=50:  59 us] [N=100:  66 us] [N=200:  84 us]
L=50: [N=2:  88 us] [N=5:  85 us] [N=10:  93 us] [N=20:  88 us] [N=50:  94 us] [N=100: 100 us] [N=200: 120 us]

The numbers after applying batch inversion:

===== Label-set approach (calculate output candidate for each label, look it up in outputs) =====
L= 1: [N=2:  38 us] [N=5:  37 us] [N=10:  37 us] [N=20:  37 us] [N=50:  41 us] [N=100:  48 us] [N=200:  61 us]
L= 2: [N=2:  35 us] [N=5:  38 us] [N=10:  37 us] [N=20:  44 us] [N=50:  41 us] [N=100:  47 us] [N=200:  62 us]
L= 3: [N=2:  35 us] [N=5:  38 us] [N=10:  39 us] [N=20:  38 us] [N=50:  46 us] [N=100:  46 us] [N=200:  66 us]
L= 5: [N=2:  49 us] [N=5:  36 us] [N=10:  36 us] [N=20:  40 us] [N=50:  42 us] [N=100:  49 us] [N=200:  63 us]
L=10: [N=2:  37 us] [N=5:  37 us] [N=10:  37 us] [N=20:  37 us] [N=50:  41 us] [N=100:  49 us] [N=200:  65 us]
L=20: [N=2:  46 us] [N=5:  47 us] [N=10:  42 us] [N=20:  42 us] [N=50:  48 us] [N=100:  56 us] [N=200:  73 us]
L=50: [N=2:  54 us] [N=5:  46 us] [N=10:  46 us] [N=20:  50 us] [N=50:  52 us] [N=100:  60 us] [N=200:  82 us]

The only code change here is using batch inversion in the label-set path.

Here is a first attempt at creating a scanning benchmark for the common case scenario (i.e. no matches and hence only one k iteration), comparing the "BIP" and "Label-set" approaches over a combination of L (number of labels to scan for) and N (number of tx outputs): theStack@fef63fd

This is now available as an actual benchmark using the framework (i.e. implemented in the module's bench_impl.h), leading to more stable results, especially for the lower-N cases (N<=10), which I'd consider to hit most frequently: theStack@8eced64, see https://gist.github.com/theStack/25c77747838610931e8bbeb9d76faf78?permalink_comment_id=5892266#gistcomment-5892266 for the results on my machine. Both approaches are now also benchmarked for no-labels scanning (i.e. L=0) and for the BIP approach, only L=1 is tested, as any higher values don't have an influence on the run-time, if we assume that the label cache lookup cost is negligible.

@w0xlt:

From my understanding, your benchmark confirms that the label-set approach performs better overall and scales more smoothly with the transaction size N (number of outputs). For small N they are comparable, but as N grows the label-set approach becomes significantly faster than the BIP approach.

I agree. It seems to me that, unless there is a really strong reasons to support use-cases with dozens of labels, we should consider switching from the BIP to the Label-set scanning approach. I'm not sure though if larger N values appear frequent enough in blocks nowadays to have a significant influence over the overall scanning time (I'd roughly guess that the vast majority of SP eligible txs wouldn't exceed 10 outputs, though that should be verified), but that could change in the future.

I also evaluated Montgomery batch inversion via secp256k1_ge_set_all_gej_var in the label-set scanning path on top of your commit. This seems to mainly help when there are many labels (large L). For L = 50 the label-set approach becomes noticeably faster still, so it dominates the BIP-style scanning for almost all (L, N) combinations in this benchmark. All runs are using the same “common case” scenario as in your example (one k iteration, no match).

That's great, didn't review this code in detail yet but it seems to be simple enough to be considered on top, if we go with the label set scanning approach; the benchmarks also look very promising, will integrate them and run on my machine as well tomorrow.